Even though it's in the xcm folder, the crate name has to include it.

Yes, agree pallet-derivatives is not a good name, pallet-xcm-derivatives, what about pallet-asset-derivatives or pallet-xcm-asset-derivatives.

Even though it's in the xcm folder

Why does it even need to be under the xcm folder? Is it really part of the xcm module?

From what I see, the pallet itself only uses a few imports from xcm-builder:

use xcm_builder::unique_instances::{
	derivatives::{DerivativesRegistry, IterDerivativesRegistry},
	DerivativesExtra,
};

For example, the DerivativesRegistry trait and most of the code in xcm-builder/src/unique_instances/derivatives.rs don’t actually use any xcm types. This could easily be moved out of the XCM modules — maybe into frame-support/traits or somewhere else.

So at first glance, I’d say this pallet is entirely XCM-agnostic.

As a comparison, pallet-revive and pallet-contracts also use xcm-builder, and they live under substrate/frame.
There are quite a lot of new traits and structs introduced in this PR — I’d prefer to do some triage/clean-up and move things around a bit.

I would add some README detailing the purpose of this and how to use it.

This is, not true? While they could be used for fee payment, they can only be used like that if they're present in a liquidity pool with DOT in the case of AH. Other chains could configure it differently and might even need them to be registered from a privileged origin, but it's not at all mandatory.

They could actually be used for fee payment. A chain might create a system with NFT coupons for free execution.

True, it could be. Though there is no real-world example at the moment, the XCM Payment API doesn't support that. Additionally, I believe there will be issues with weight refunds.
But yeah, still possible. I need to reformulate this.

Why do you want anyone to create a derivative NFT representing any real remote NFT?

Moving NFT assets across chains securely via XCM is a very innovative capability (bridges aside), and use cases for it are basically unknown.
In such a case, the loose permissions should encourage the development of use cases, which can then drive the limitations that should be put on rights to create the derivative collection. The capabilities to accomplish that technically will grow over time, and it will become easier to implement the fine-tuned rights management if the need arises. At this stage, the most important thing is to get use cases in the first place. Therefore, it seems there is no need to create extra limitations at this moment.

In the future, we could add the following safeguards:

1. create_derivative will only start the registration process, sending the ReportMetadata XCM instruction to the corresponding chain and registering a handler-extrinsic that will finalize the registration.
2. The handler-extrinsic can check the specific metadata fields and decide if it is okay to register such a derivative collection.
3. Also, the reserve chain could decide not to report the metadata, effectively forbidding the creation of a derivative collection (e.g., such a chain could provide a way for a collection's owner to forbid metadata reporting).

This way, the registration process will be very simple without the need to involve any voting process, yet it will be secure. But given that XCM Asset Metadata isn't here yet, I'd suggest starting to experiment with a simple unprivileged registration.

@mrshiposha I see that you’ve put quite a bit of work into this — it’s really cool, and I like it. I’d definitely like to support you on this. To move forward, let’s start by moving pallet-derivatives out of the XCM subfolder - for example, to polkadot-sdk/substrate/frame/derivatives, or if it’s specifically about NFTs, then to polkadot-sdk/substrate/frame/nfts/derivatives:

• It currently doesn’t provide any XCM functionality (yet). If I understand correctly, it’s more of a FRAME-level pallet used to store some mappings, without any cross-chain or consensus-related functionality.
• Keeping it under the XCM folder could lead to the false assumption that the XCM team maintains or supports it.
• As for the Uniques AssetTransactor integration supporting XCM assets/instructions — that can stay in xcm-builder.

I’d suggest we start there 👆 , and then revisit the other traits/stuff added.

In the future, we could add the following safeguards:
create_derivative will only start the registration process, sending the ReportMetadata XCM instruction to the corresponding..

Exactly. Then we can add a type SendXcm to the pallet-derivatives, or introduce some other trait that will craft an XCM program. Again, this aligns with my earlier comments that pallet-derivatives doesn’t belong under the XCM modules. I still see pallet-derivatives as a kind of utility pallet to support NFT-related functionality, not as an XCM module.

Do we want to handle NFT transfers via XCM? That would be great! What are we missing to make that happen? Do we need new XCM instruction(s)? If so, why not start by defining them - perhaps through an XCM RFC? We should begin with a clear narrative and use cases, and/or propose an RFC for any new instructions that would be needed.

I’d suggest we start there 👆 , and then revisit the other traits/stuff added.

Will do!

Do we need new XCM instruction(s)?

The current instruction set is enough for transferring NFTs. At least via reserve-based transfers, since they just transfer the "ownership" without relocating the original object, while teleports require the metadata transfer. The data transfer is addressed by the XCM Asset Metadata RFC: https://polkadot-fellows.github.io/RFCs/approved/0125-xcm-asset-metadata.html.

The only thing we need are clear and general tools for making AssetTransactors for the existing NFT solutions.

It's just that NFT implementations are diverse because there could be a lot of aspects to the unique objects: different ID assignment approaches and metadata management, such as whether to destroy the metadata on destroying an NFT or not. The ecosystem already has different implementations: pallet-nfts, pallet-uniques, Unique Network, ORML NFTs, Aventus NFTs. Each of them follows their own path. There are similarities as well, of course, but generally they don't fall into "one big NFT interface".

We should begin with a clear narrative and use cases, and/or propose an RFC for any new instructions that would be needed.

Hmm. Maybe I should revisit my Sub0 presentations (note that at the time this was an early concept) Lisbon 2023, Bangkok 2024 and make a corresponding text that will include the "big picture," including the future XCM Asset Metadata instruction uses.

Why might you want to not store the mapping? Because the ids are the same? Can you add that to this doc comment?

Yes, when IDs are the same, we don't need a mapping. In this scenario, the pallet only provides an API for derivative creation/destruction.

I'll highlight this in the doc comment

please add some docs here for pub stuff

Yes, agree pallet-derivatives is not a good name, pallet-xcm-derivatives, what about pallet-asset-derivatives or pallet-xcm-asset-derivatives.

Even though it's in the xcm folder

Why does it even need to be under the xcm folder? Is it really part of the xcm module?

From what I see, the pallet itself only uses a few imports from xcm-builder:

use xcm_builder::unique_instances::{
	derivatives::{DerivativesRegistry, IterDerivativesRegistry},
	DerivativesExtra,
};

For example, the DerivativesRegistry trait and most of the code in xcm-builder/src/unique_instances/derivatives.rs don’t actually use any xcm types. This could easily be moved out of the XCM modules — maybe into frame-support/traits or somewhere else.

So at first glance, I’d say this pallet is entirely XCM-agnostic.

As a comparison, pallet-revive and pallet-contracts also use xcm-builder, and they live under substrate/frame.
There are quite a lot of new traits and structs introduced in this PR — I’d prefer to do some triage/clean-up and move things around a bit.

First of all, I don't like exposing DispatchResult and DispatchError in the XCM traits here. If you look at the DispatchError variants, which one would you use for try_register_derivative? In most cases, you'll likely end up probably using DispatchError::Other. XCM modules should be as independent of FRAME-specific types as possible. (I know we use some - for example, xcm-builder only uses DispatchResult for testing salary - which I also think should be removed and replaced also).

XCM processing isn't about dispatchables; it's about interpreting and executing XCM programs and instructions. So if this logic is truly XCM-related, you should use something like Result<T, XcmError> instead.

In fact, the DerivativesRegistry and related logic seem more FRAME-oriented and could probably be moved out of the XCM module altogether, along with the pallet derivatives, as I mentioned earlier.

/// This is the legacy return type of `Dispatchable`. It is still exposed for compatibility reasons.
/// The new return type is `DispatchResultWithInfo`. FRAME runtimes should use
/// `frame_support::dispatch::DispatchResult`.
pub type DispatchResult = core::result::Result<(), DispatchError>;

/// Reason why a dispatch call failed.
pub enum DispatchError {

If I understand correctly, nothing from the file polkadot/xcm/xcm-builder/src/unique_instances/derivatives.rs is used by either polkadot/xcm/xcm-builder/src/unique_instances/adapter.rs or polkadot/xcm/xcm-builder/src/unique_instances/ops.rs. So I’d say the entire derivatives.rs file can be cleanly moved into pallet-derivatives.

Once pallet-derivatives is moved to substrate/frame (as previously suggested), you can also properly take advantage of using DispatchResult and DispatchError. I also think that polkadot/xcm/xcm-builder/src/unique_instances/ops.rs could be moved directly into pallet-derivatives as well.

I would really recommend splitting this PR into the parts:

• XCM-related components
  • (From what I see, it's mainly XCM asset transacting — that can stay in xcm-builder as an implementation of TransactAsset.)
• Components related to the configuration and logic of pallet-derivatives
  • This includes most (if not all) of the code in these files (unique_instances/ops.rs, unique_instances/derivatives.rs`
  • For example, see this part from your mock file:
    https://github.com/UniqueNetwork/polkadot-sdk/blob/5ee9516eb4239399f1dedba4db9411fe0f17a280/polkadot/xcm/pallet-derivatives/src/mock/mod.rs#L71-L75

I think all this stuff around FallibleConvert is not really necessary, and everything what is needed can be done with existing tools e.g. TryConvert / TryConvertBack.

For example, if you use TryConvertBack, then we don't need two OriginalToDerivativeConvert and DerivativeToOriginalConvert - you can have just one OriginalToDerivativeConvert which implements both TryConvert and TryConvertBack - this way + removing FallibleConvert - lots of code can go away, which just simplifies the stuff.

For FallibleConvert, if the conversion fails, which variant are you planning to use?

pub enum DispatchError {
	/// Some error occurred.
	Other(
		#[codec(skip)]
		#[cfg_attr(feature = "serde", serde(skip_deserializing))]
		&'static str,
	),
	/// Failed to lookup some data.
	CannotLookup,
	/// A bad origin.
	BadOrigin,
	/// A custom error in a module.
	Module(ModuleError),
	/// At least one consumer is remaining so the account cannot be destroyed.
	ConsumerRemaining,
	/// There are no providers so the account cannot be created.
	NoProviders,
	/// There are too many consumers so the account cannot be created.
	TooManyConsumers,
	/// An error to do with tokens.
	Token(TokenError),
	/// An arithmetic error.
	Arithmetic(ArithmeticError),
	/// The number of transactional layers has been reached, or we are not in a transactional
	/// layer.
	Transactional(TransactionalError),
	/// Resources exhausted, e.g. attempt to read/write data which is too large to manipulate.
	Exhausted,
	/// The state is corrupt; this is generally not going to fix itself.
	Corruption,
	/// Some resource (e.g. a preimage) is unavailable right now. This might fix itself later.
	Unavailable,
	/// Root origin is not allowed.
	RootNotAllowed,
	/// An error with tries.
	Trie(TrieError),
}

For FallibleConvert, if the conversion fails, which variant are you planning to use?

The Module variant. The FallibleConvert is supposed to propagate pallet errors.

For instance, as it is used in the DeriveStrategyThenCreate type.
The NFT traits (Create, Update, Destroy, etc.) are supposed to usually be called from an extrinsic context, so they return a DispatchError when something goes wrong (containing a Module error since it will always be some pallet that implements the corresponding NFT operation).
The FallibleConvert's purpose is to return a proper pallet error, so we could return it inside an NFT operation.

For example, in the pallet tests, the CreateDerivativeOwnedBySovAcc instance will return the pallet_derivatives::Error::InvalidAsset error if there is no way to get a sovereign account for the given asset ID, and the error will be propagated through the create_derivative extrinsic back to the client.
Also, if an error occurs inside the NFT engine, it will be propagated as well.

Another util type where FallibleConvert is used is MapId. FallibleConvert helps to avoid using DispatchError::Other inside MapId's NFT traits implementations.
In the pallet tests, the OriginalToDerivativeConvert is used where FallibleConvert is needed.
(However, I acknowledge that in the XCM context, it is of little use since if an error occurs, it will be converted to FailedToTransactAsset anyway. But it could be useful if this were called from an extrinsic, not from an XCM context, as in the case above).

I'm all for improving the error reporting and don't mind refactoring this. I just want to be able to propagate the pallet errors in NFT operations so that extrinsics where they are called could report the proper errors (especially when operations are nested under other pallets' APIs, such as pallet-nft-fractionalization).

@mrshiposha please use better asset_ok instead of unwrap where possible

@mrshiposha please also check all other files, they are missing headers (licenses and stuff)

Usually, from what I’ve seen, a lot of conversions — especially in the XCM module - simply return the original A value as an error when they fail. So in such cases, you don’t really need a FallibleConvert.

If we wanted to go deeper, we could introduce something more generic (and arguably over-complicated), like:

pub trait FallibleConvert<A, B, E> {
	/// Attempt to perform the conversion.
	fn fallible_convert(a: A) -> Result<B, E>;
}

But then, when you have something like SomeImplFallibleConvert<A, B, DispatchResult> and/or OtherImplFallibleConvert<A, B, XcmError>, you’d need to deal with error conversions and additional boilerplate. Let’s not overthink it unless it’s truly necessary.

That said, I’d definitely start with the cleanup here:
https://github.com/paritytech/polkadot-sdk/pull/4300/files#r2160173529

And after that, maybe we will see that this FallibleConvert can also go in the pallet-derivatives.